Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation for zone in favour of explicit zone_id #421

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

jacobbednarz
Copy link
Member

Zone names within Cloudflare are not universally unique which makes
performing a lookup based on just the name a little troublesome. Two
accounts can have the same zone name and both of these can be returned
if the lookups are not restricted to the zone ID or account ID.

To rememdy this, zone is being deprecated in favour of explicit
zone_id values. See the discussion on this for full details.

Following Terraform's guidelines for deprecations, we are putting in
a deprecation notice as part of a minor release and then in our next
major release, we will actually perform the schema and functionality
removal. This update will not prevent people from using zone however
use will trigger a warning that it's going away and they should migrate
themselves.

Zone names within Cloudflare are not universally unique which makes
performing a lookup based on just the name a little troublesome. Two
accounts can have the same zone name and both of these can be returned
if the lookups are not restricted to the zone ID or account ID.

To rememdy this, `zone` is being deprecated in favour of explicit
`zone_id` values. See the discussion[1] on this for full details.

Following Terraform's guidelines for deprecations[2], we are putting in
a deprecation notice as part of a minor release and then in our next
major release, we will actually perform the schema and functionality
removal. This update will not prevent people from using `zone` however
use will trigger a warning that it's going away and they should migrate
themselves.

[1]: https://github.com/terraform-providers/terraform-provider-cloudflare/issues/161#issuecomment-441128985
[2]: https://www.terraform.io/docs/extend/best-practices/deprecations.html#provider-attribute-removal
@jacobbednarz jacobbednarz force-pushed the add-deprecation-for-zone-id branch from af8fe8a to c75c477 Compare July 18, 2019 22:24
@ghost ghost added size/M and removed size/XS labels Jul 18, 2019
@jacobbednarz
Copy link
Member Author

Confirmed this is working for a tested resource.

$ terraform apply

Warning: cloudflare_filter.wordpress: "zone": [DEPRECATED] `zone` is deprecated in favour of explicit `zone_id` and will be removed in the next major release


Warning: cloudflare_firewall_rule.wordpress: "zone": [DEPRECATED] `zone` is deprecated in favour of explicit `zone_id` and will be removed in the next major release

@patryk patryk merged commit 516b1d5 into cloudflare:master Jul 19, 2019
@patryk
Copy link
Contributor

patryk commented Jul 19, 2019

🥽

@SteveGoldthorpe-Work
Copy link
Contributor

Think we've been a bit premature with this.
pagerule with zone specified:

Warning: "zone": [DEPRECATED] `zone` is deprecated in favour of explicit `zone_id` and will be removed in the next major release

  on pagerules_xxx.tf line 3, in resource "cloudflare_page_rule" "my_page_rule":
   3: resource "cloudflare_page_rule" "my_page_rule" {

pagerule with zone_id specified:

Error: Missing required argument

  on pagerules_xxx.tf line 3, in resource "cloudflare_page_rule" "my_page_rule":
   3: resource "cloudflare_page_rule" "my_page_rule" {

The argument "zone" is required, but no definition was found.

pagerule with both zone and zone_id specified:

Error: "zone_id": this field cannot be set

  on pagerules_xxx.tf line 3, in resource "cloudflare_page_rule" "my_page_rule":
   3: resource "cloudflare_page_rule" "my_page_rule" {

Looking in resource_cloudflare_page_rule.go, it seems zone is still Required and zone_id is computed (hence the above). We probably shouldn't add deprecation warnings until we can take both the new AND old fields. Maybe set them both to Optional and have a check at least one (or only one if both wouldn't be valid) is set?
Terraform v0.12.6

  • provider.cloudflare v1.17.0

@patryk
Copy link
Contributor

patryk commented Aug 9, 2019

Good catch, @SteveGoldthorpe-Work. We should make zone_id settable. Forked to a new issue.

patryk added a commit that referenced this pull request Aug 9, 2019
jacobbednarz added a commit to jacobbednarz/terraform-provider-cloudflare that referenced this pull request Aug 21, 2019
We have been working to perform some breaking changes in cloudflare#227 and the
first attempt at this in cloudflare#421 neglected to allow people to set `zone_id`
and action the deprecation notice 🤦

This second attempt has the same goal of deprecating `zone` in favour of
explicit `zone_id` however this time I've updated both schema properties
to be optional and added some validation in the various `Create` methods
to ensure that we get at least one of them during the swap over period.
That code can be removed as soon we've cut a breaking change release.

Closes cloudflare#439
patryk pushed a commit that referenced this pull request Aug 22, 2019
We have been working to perform some breaking changes in #227 and the
first attempt at this in #421 neglected to allow people to set `zone_id`
and action the deprecation notice 🤦

This second attempt has the same goal of deprecating `zone` in favour of
explicit `zone_id` however this time I've updated both schema properties
to be optional and added some validation in the various `Create` methods
to ensure that we get at least one of them during the swap over period.
That code can be removed as soon we've cut a breaking change release.

Closes #439
boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants